-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CF-106] Fetch AdServices Token #1519
Conversation
…ction, deprecated automaticAppleSearchAdsAttributionCollection
RevenueCat.xcodeproj/project.pbxproj
Outdated
@@ -304,6 +304,7 @@ | |||
A563F586271E072B00246E0C /* MockBeginRefundRequestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = A563F585271E072B00246E0C /* MockBeginRefundRequestHelper.swift */; }; | |||
A563F589271E1DAD00246E0C /* MockBeginRefundRequestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = A563F585271E072B00246E0C /* MockBeginRefundRequestHelper.swift */; }; | |||
A56F9AB126990E9200AFC48F /* CustomerInfo.swift in Sources */ = {isa = PBXBuildFile; fileRef = A56F9AB026990E9200AFC48F /* CustomerInfo.swift */; }; | |||
A5B6CDD8280F3843007629D5 /* AdServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = A5B6CDD5280F3843007629D5 /* AdServices.framework */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be weakly linked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case I don't get to review this again, just wanted to point out that this is probably my most critical comment. I believe this has to be weakly linked so we don't make all users of the SDK implicitly link AdServices
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this, this part is a very delicate change that we should research and test out thoroughly.
It might even be a red flag for privacy-minded folks who're looking into using our SDK. Perhaps we can do the extra work of calling this through the same "reflection" we did for iAd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you both okay with me doing the linking research as the follow-up ticket i created? would definitely be before merging into main. this just feels like enough work to warrant a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely! It's only important to get the linking right before merging to main, but it can totally be done as a separate thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay then that's the plan!
@@ -82,6 +86,21 @@ class AttributionFetcher { | |||
#endif | |||
} | |||
|
|||
@available(iOS 14.3, *) | |||
func adServicesToken(completion: @escaping (String?, Error?) -> Void) { | |||
// TODO check for library? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think canImport
would help here too :)
@@ -71,6 +73,10 @@ extension StoreKitStrings: CustomStringConvertible { | |||
case .sk1_no_known_product_type: | |||
return "This StoreProduct represents an SK1 product, the type of product cannot be determined, " + | |||
"the value will be undefined. Use `StoreProduct.productCategory` instead." | |||
|
|||
case .unknown_sk2_product_discount_type(let rawValue): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
Sources/Purchasing/Purchases.swift
Outdated
*/ | ||
@objc public static var automaticAppleSearchAdsAttributionCollection: Bool = false | ||
@available(iOS 14.3, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will need the tvOS
/macCatalyst
(macOS
too?) equivalents too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep good call. xcode never suggests the full checks for us, do they...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, only for the currently selected target unfortunately.
Sources/Purchasing/Purchases.swift
Outdated
* Enable automatic collection of Apple Search Ads attribution. Defaults to `false`. | ||
*/ | ||
@available(*, deprecated, message: "use Purchases.automaticAdServicesAttributionTokenCollection for iOS 14.3+ instead") | ||
@objc static var automaticAppleSearchAdsAttributionCollection: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this to Deprecations.swift
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a difference between methods in Deprecations.swift and those in the // MARK: Deprecated section of Purchases.swift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't realize we still had some there. The idea was to make the main file (which is already huge) smaller by only having the current API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to me! i can move the rest while i'm at it...
Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>
2abd531
to
160efb4
Compare
expect(MockAdClientProxy.requestAttributionDetailsCallCount) == 0 | ||
} | ||
|
||
func testPostAppleSearchAdsAttributionIfNeededSkipsIfAlreadySent() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll add back related tests in the post PR
need to figure out why build-tv-watch-and-macos is failing, but otherwise this is ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor comments.
Let me know if I can help figuring out the watchOS issue, might be because AdServices
isn't available, so maybe weak linking might fix that?
#else | ||
completion(nil, AttributionFetcherError.identifierForAdvertiserUnavailableForPlatform) | ||
Logger.warn(Strings.attribution.adservices_not_supported) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
return | ||
// should match OS availability in https://developer.apple.com/documentation/ad_services | ||
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) | ||
func adServicesToken() -> String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better as a property:
func adServicesToken() -> String? { | |
var adServicesToken: String? { |
if let attributionToken = try? AAAttribution.attributionToken() { | ||
return attributionToken | ||
} else { | ||
Logger.warn(Strings.attribution.adservices_token_fetch_failed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe do a do/catch instead of try?
to add the underlying error to this?
Also I wonder if it should be Logger.appleError
Sources/Purchasing/Purchases.swift
Outdated
postAppleSearchAddsAttributionCollectionIfNeeded() | ||
|
||
// should match OS availability in https://developer.apple.com/documentation/ad_services | ||
if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) { postAdServicesTokenIfNeeded() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting here is weird, did you mean to put it in a new line?
if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) { postAdServicesTokenIfNeeded() | |
if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) { | |
postAdServicesTokenIfNeeded() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh woops yes. was on a tiny screen this day and didn't realize, good catch
@@ -259,7 +259,7 @@ extension StoreProductDiscount.DiscountType { | |||
case SK2ProductDiscount.OfferType.promotional: | |||
return .promotional | |||
default: | |||
Logger.warn(Strings.attribution.unknown_sk2_product_discount_type(rawValue: sk2Discount.type.rawValue)) | |||
Logger.warn(Strings.storeKit.unknown_sk2_product_discount_type(rawValue: sk2Discount.type.rawValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
@@ -7,6 +7,8 @@ | |||
|
|||
class MockAttributionFetcher: AttributionFetcher { | |||
|
|||
var adServicesTokenCollectionCalled = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe move this right on top of adServicesToken
? I think that's what other mocks do
@@ -1,16 +0,0 @@ | |||
{ | |||
"headers" : { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing these too!
Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>
that was me trying to weak link the framework. i'm not seeing very clear guidance on how to do that in an initial google search... |
if you have time to look that'd be great 🙏 |
@@ -7,6 +7,7 @@ | |||
objects = { | |||
|
|||
/* Begin PBXBuildFile section */ | |||
2C396F5E281C64B700669657 /* AdServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 2C396F5D281C64B700669657 /* AdServices.framework */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APITester
s needs it is a sign that users will need to add it as well, which isn't good. I'm guessing the actual problem was this: #1554.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. promise i won't forget this before merging to main.
This fixes the watchOS and tvOS compilation (yay CI test coverage catching this!) Now the framework is only linked for the rest of the platforms.
@NachoSoto thanks for your help!! i think this finally passed everything if you have time to review :) |
self.post(attributionData: attributionDetails, fromNetwork: .appleSearchAds, networkUserId: nil) | ||
} | ||
Logger.debug("Logging attribution token for now to avoid lint warning: \(attributionToken)") | ||
// post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave this as a TODO
? it's okay if lint fails since we're not merging this in main
yet. That way we ensure it doesn't slip through the cracks.
*/ | ||
@objc public static var automaticAppleSearchAdsAttributionCollection: Bool = false | ||
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being implicitly marked as available for any version of tvOS
and watchOS
. You need to add this:
@available(tvOS, unavailable)
@available(watchOS, unavailable)
private func postAppleSearchAddsAttributionCollectionIfNeeded() { | ||
guard Self.automaticAppleSearchAdsAttributionCollection else { | ||
// should match OS availability in https://developer.apple.com/documentation/ad_services | ||
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops missed this, will get into the post PR
* [CF-106] Fetch AdServices Token (#1519) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com> * [CF-553] Post AdServices token (#1534) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com> * Update ios 14 snapshots tets * Reworked adservices to use new Purchases.shared.attribution API * Updated with some comments from PR * Add iAd code back (#1739) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> * use new mapKeys dictionary extension for device cache * update for some pr comments * remove purchasestests * generate missing snapshots * fix deprecation messages to clarify method now called on shared instance * Fix for inserting nonstring into userdefaults * Add tests for bug * fix lint * Added availablility checks for tests and removed unused varialbes * Whoops... wrong class Co-authored-by: beylmk <maddie@revenuecat.com> Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>
AdServices token fetching and removal of old iAD code. Posting to the backend will be the next step.
There is a follow-up ticket to investigate linking, and we will probably switch to weak-linking then.